Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use direct path in testpoolswap when available #1115

Merged
merged 4 commits into from
Mar 9, 2022

Conversation

Jouzo
Copy link
Collaborator

@Jouzo Jouzo commented Feb 22, 2022

What kind of PR is this?:

/kind fix

What this PR does / why we need it:

Uses direct path in testpoolswap when available

Which issue(s) does this PR fixes?:

Fixes #1114

Bushstar
Bushstar previously approved these changes Feb 22, 2022
@eli-lim
Copy link

eli-lim commented Feb 22, 2022

Just curious, which of the following are we looking to achieve for testpoolswap?

  1. Always use direct path if available
  2. Use best path (implying some composite path may yield an amount greater than direct - assuming that's possible)

Asking because if (2) is preferred, the logic could instead be in CPoolSwap::CalculatePoolPaths.

@Jouzo
Copy link
Collaborator Author

Jouzo commented Feb 22, 2022

Just curious, which of the following are we looking to achieve for testpoolswap?

1. Always use direct path if available

2. Use _best_ path (implying some composite path may yield an amount greater than direct - assuming that's possible)

Asking because if (2) is preferred, the logic could instead be in CPoolSwap::CalculatePoolPaths.

Good question. Here I addressed the issue by going for the first option but that might not always be the best scenario.
Another option would be to add "composite" to the accepted path values which would always test for compositeswap.
Then we would have:

  • direct: Always test for a poolswap
  • composite: Always test for a compositeswap
  • auto: Use a direct swap via poolswap if available (as currently in this PR) or go through compositeswap
  • [poolpairs_ids]: Try through a custom path

With this additional path, a user could try for direct vs composite and choose the best of two paths (scenario 2).

surangap
surangap previously approved these changes Feb 23, 2022
@surangap
Copy link
Contributor

Just curious, which of the following are we looking to achieve for testpoolswap?

1. Always use direct path if available

2. Use _best_ path (implying some composite path may yield an amount greater than direct - assuming that's possible)

Asking because if (2) is preferred, the logic could instead be in CPoolSwap::CalculatePoolPaths.

Good question. Here I addressed the issue by going for the first option but that might not always be the best scenario. Another option would be to add "composite" to the accepted path values which would always test for compositeswap. Then we would have:

  • direct: Always test for a poolswap
  • composite: Always test for a compositeswap
  • auto: Use a direct swap via poolswap if available (as currently in this PR) or go through compositeswap
  • [poolpairs_ids]: Try through a custom path

With this additional path, a user could try for direct vs composite and choose the best of two paths (scenario 2).

saw this later. I guess it could be possible that a composite swap yields more than the direct swap 🤔
I think the main problem is that CalculateSwaps is not able to handle the scenario where only the direct pool is available and no other path. need to write a unit test and confirm it.

@eli-lim
Copy link

eli-lim commented Feb 23, 2022

Just curious, which of the following are we looking to achieve for testpoolswap?

1. Always use direct path if available

2. Use _best_ path (implying some composite path may yield an amount greater than direct - assuming that's possible)

Asking because if (2) is preferred, the logic could instead be in CPoolSwap::CalculatePoolPaths.

Good question. Here I addressed the issue by going for the first option but that might not always be the best scenario. Another option would be to add "composite" to the accepted path values which would always test for compositeswap. Then we would have:

  • direct: Always test for a poolswap
  • composite: Always test for a compositeswap
  • auto: Use a direct swap via poolswap if available (as currently in this PR) or go through compositeswap
  • [poolpairs_ids]: Try through a custom path

With this additional path, a user could try for direct vs composite and choose the best of two paths (scenario 2).

composite here would mean "use the best possible composite swap path"? I would think that's a pretty niche use-case 😮 (ofc I could be wrong haha).
Regarding the user trying both options - we probably want to avoid having to make 2 RPCs as it would be a performance / UX impediment for wallet users. So since the user is mainly concerned with "the best option", then we should just give em that straightaway -> this is an existing use-case in defichain wallet (BirthdayResearch/jellyfishsdk#1065)

saw this later. I guess it could be possible that a composite swap yields more than the direct swap 🤔 I think the main problem is that CalculateSwaps is not able to handle the scenario where only the direct pool is available and no other path. need to write a unit test and confirm it.

Agree. Deeper - CalculatePoolPaths doesn't ever consider a direct path. So we could just have the logic there instead.
Then auto would return the "best" among the direct and possible composite paths, since the direct path is also included in the CalculateSwaps evaluation.

@Jouzo
Copy link
Collaborator Author

Jouzo commented Feb 23, 2022

The logic behind it was that we have two different RPCs for poolswap and compositeswap.
In that case, testpoolswap composite would give the result of a RPC compositeswap call while testpoolswap direct would give the result of a RPC poolswap call.

@eli-lim
Copy link

eli-lim commented Feb 23, 2022

The logic behind it was that we have two different RPCs for poolswap and compositeswap. In that case, testpoolswap composite would give the result of a RPC compositeswap call while testpoolswap direct would give the result of a RPC poolswap call.

Ah I see what you mean. I think just fixing the "auto" path to give the best swap (be it a direct or composite) would suffice to fix the issue at BirthdayResearch/jellyfishsdk#1065.

Regarding the 'composite' option - really depends if ain would like to support this haha but there's no immediate use-case for it I believe

@Jouzo Jouzo dismissed stale reviews from surangap and Bushstar via 8c38863 March 2, 2022 13:32
@prasannavl prasannavl added 2.6.2 and removed 2.7.0 labels Mar 9, 2022
@prasannavl prasannavl merged commit 3c2ec74 into master Mar 9, 2022
@prasannavl prasannavl deleted the fix/direct_testpoolswap branch March 9, 2022 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

testpoolswap RPC should check for a direct swap first when auto is specified.
6 participants